-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Default semantic_text fields to ELSER on EIS when available #134708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Default semantic_text fields to ELSER on EIS when available #134708
Conversation
|
Hey @ioanatia! 👋 Just implemented the dynamic ELSER default selection for semantic_text fields. The approach is intentionally minimal -leverages existing A few things I'd especially appreciate feedback on:
Still draft mode, but wanted to get your input before finalizing. Thanks! |
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Outdated
Show resolved
Hide resolved
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Outdated
Show resolved
Hide resolved
|
relying on but I think we need more tests. |
|
In terms of tests, what we can also do is to add another mock service in https://github.com/elastic/elasticsearch/tree/main/x-pack/plugin/inference/qa/test-service-plugin/src/main/java/org/elasticsearch/xpack/inference/mock and then use it in yaml tests. the InferenceService allows to register default endpoints: elasticsearch/server/src/main/java/org/elasticsearch/inference/InferenceService.java Line 229 in 66582a3
I think we can mock the default endpoints we have in EIS and use the same name in the mock inference service. |
...rc/main/java/org/elasticsearch/xpack/inference/services/elastic/ElasticInferenceService.java
Show resolved
Hide resolved
Thanks @Mikep86! All comments addressed. Have verified about the default assertion in the SemanticTextEISDefaultIT, there is no flakiness or underlying issue at the moment. Since we're holding off on merging due to the ELSER rate limiting work, no rush on further review. Take your time! 😊 |
...nce/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java
Outdated
Show resolved
Hide resolved
...e-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/InferenceSemanticTextIT.java
Outdated
Show resolved
Hide resolved
...-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/SemanticTextEISDefaultIT.java
Show resolved
Hide resolved
|
@ioanatia @Mikep86 quick update: I’ve addressed the review feedback and pushed the latest changes. |
Mikep86
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM to merge once we get the 👍 from @seanhandley and @maxjakob
...-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/SemanticTextEISDefaultIT.java
Show resolved
Hide resolved
|
@mridula-s109 , do we have a separate PR to update the docs?
|
|
Great work @mridula-s109 ! We've decided to wait anther week while we investigate a bug report on our side. Once it's mitigated we can hopefully merge this. I've pushed our calendar reminder back to next week to accommodate this. |
Thanks for the update @seanhandley! 👍 No problem at all, waiting another week makes sense to ensure everything is stable. I'll keep an eye out for next week's calendar reminder. In the meantime, @liranabn raised a good point about doc updates. I'll use this week to add the documentation changes directly to this PR (updating the section that mentions |
Are there any other doc pages or files that reference the |
|
Thanks @leemthompo for the inputs, i will look into it! |
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Show resolved
Hide resolved
|
@mridula-s109 We're clear to merge this today 👍 There are other pieces of work we want to do but none of them are blocking this from being merged. |
Thanks for letting me know @seanhandley, As the CI has passed as well post updating from main today, i will go ahead with the merge now. |
|
Opening up a followup PR to modify the SemanticFieldMappertests to point to the new constant in SemanticTextFieldMapper.java. |
…#134708) * Defaulting EIS on ELSER * Extended testing * [CI] Auto commit changes from spotless * Edited to include the default * Cleaned up the mapper implementation * COmpile issue * [CI] Auto commit changes from spotless * Added tests * [CI] Auto commit changes from spotless * Refactored the variable names * Cleanup done * Removed unnecessary files * Unit tests and mock is working * [CI] Auto commit changes from spotless * Fix test * yaml addition failure * [CI] Auto commit changes from spotless * Resolved the import issue or duplication of variables in mock * Resolved PR comments * Restored error * Update docs/changelog/134708.yaml * Integration test * [CI] Auto commit changes from spotless * Resolved all PR comments * [CI] Auto commit changes from spotless * Cleaned up the redudant reference of TestInferencePlugin * Included both before and before test * [CI] Auto commit changes from spotless * Made changes to accomodate the old constant version string --------- Co-authored-by: elasticsearchmachine <[email protected]>
🚨 Please check with search inference team on when to merge this - we're working out some rate limiting issues ahead of GA of ELSER on EIS
Summary
Implements dynamic default selection for semantic_text fields to automatically use ELSER on EIS
(
.elser-2-elastic) when available, with graceful fallback to ML nodes (.elser-2-elasticsearch).Problem
Currently, semantic_text fields are hardcoded to default to
.elser-2-elasticsearch(ML nodes). This missesopportunities for better performance and cost efficiency when EIS is available.
Solution
ModelRegistry.containsDefaultConfigId()to detect EIS availability.elser-2-elasticwhen available, falls back to.elser-2-elasticsearchChanges
getPreferredElserInferenceId()method for dynamic selection logicTesting
Impact